WIP: AXI_XBAR parameters #157

WRoenninger · 2021-01-26T11:04:35Z

This is a first WIP to update axi_xbar according to #153.

I also added and adapted the documentation that was in doc/axi_xbar.md for inline for auto generation.
The doc file therefore should probably be removed when this is merged to prevent documentation duplication.

Renamed ports and paramters of axi_xbar, added inline documentation.
Added parameter sweep to the xbar portion of scripts/run_vsim.sh
Renamed TB parameters / added appropriate macros for assign
- Changed wrong transfer warnings in tb_axi_xbar_pkg to errors

The axi_pkg::xbar_cfg_t struct is still in as it is currently used in axi_lite_xbar also and can be removed when its ports are updated.

Edit: Also when looking at https://pulp-platform.github.io/axi/xbar_parameters/module.axi_xbar.html the link to doc/axi_xbar.png is currently broken. How do I link properly tho the image in the inline documentation?

andreaskurth · 2021-01-29T19:09:02Z

Very nice, thanks already!

Edit: Also when looking at https://pulp-platform.github.io/axi/xbar_parameters/module.axi_xbar.html the link to doc/axi_xbar.png is currently broken. How do I link properly tho the image in the inline documentation?

You need to move axi_xbar.png into docs/ folder (does not yet exist on master). You can then specify a path relative to that folder.

andreaskurth

Essentially LGTM in a high-level review, thanks a lot!

andreaskurth · 2021-01-29T19:11:33Z

src/axi_xbar.sv

+  /// [in flight](doc/#in-flight) at the same time.
+  parameter int unsigned SlvPortMaxTxns = 32'd0,
+  /// Maximum number of open transactions each slave connected to the crossbar can have
+  /// [in flight](../doc#in-flight) per ID at the same time.


Either this link or the one above is off, no?

Seems that both are not working. So the referenced document also has to go into the docs/ folder?

Ah, I see; because the link should go to https://github.com/pulp-platform/axi/tree/master/doc#in-flight, which is part of the repository, not of the documentation tree.

What we probably want is to add this information to the index document of the documentation. That is, https://pulp-platform.github.io/axi/master/index.html should contain a Terminology section, to which we can then link. The relative reference to the definition in the index would be index#in-flight.

src/axi_xbar.sv

andreaskurth · 2021-02-01T08:19:17Z

src/axi_pkg.sv

  /// Configuration for `axi_xbar`.
  typedef struct packed {
+    /// Number of slave ports of the crossbar.
+    /// This many master modules are connected to it.
    int unsigned   NoSlvPorts;
+    /// Number of master ports of the crossbar.
+    /// This many slave modules are connected to it.
    int unsigned   NoMstPorts;
+    /// Maximum number of open transactions each master connected to the crossbar can have in
+    /// flight at the same time.
    int unsigned   MaxMstTrans;
+    /// Maximum number of open transactions each slave connected to the crossbar can have in
+    /// flight at the same time.
    int unsigned   MaxSlvTrans;
+    /// Determine if the internal FIFOs of the crossbar are instantiated in fallthrough mode.
+    /// 0: No fallthrough
+    /// 1: Fallthrough
    bit            FallThrough;
+    /// The Latency mode of the xbar. This determines if the channels on the ports have
+    /// a spill register instantiated.
+    /// Example configurations are provided with the enum `xbar_latency_e`.
    xbar_latency_e LatencyMode;
+    /// AXI ID width of the salve ports. The ID width of the master ports is determined
+    /// Automatically. See `axi_mux` for details.
    int unsigned   AxiIdWidthSlvPorts;
+    /// The used ID portion to determine if a different salve is used for the same ID.
+    /// See `axi_demux` for details.
    int unsigned   AxiIdUsedSlvPorts;
+    /// AXI4+ATOP address field width.
    int unsigned   AxiAddrWidth;
+    /// AXI4+ATOP data field width.
    int unsigned   AxiDataWidth;
+    /// The number of address rules defined for routing of the transactions.
+    /// Each master port can have multiple rules, should have however at least one.
+    /// If a transaction can not be routed the xbar will answer with an `axi_pkg::RESP_DECERR`.
    int unsigned   NoAddrRules;
  } xbar_cfg_t;


This struct can be removed, right?

Still used in axi_lite_xbar.

WRoenninger · 2021-02-01T09:35:16Z

Added the docs/ folder and copied the axi_xbar.svg/.png to it.
Are the now redundant files for the axi_xabr documentation save to delete from the doc/ folder with this PR?

andreaskurth · 2021-02-01T09:38:36Z

Are the now redundant files for the axi_xabr documentation save to delete from the doc/ folder with this PR?

Yes

WRoenninger · 2021-02-01T09:41:27Z

Seems however that the link to the .png in line 28 to docs/axi_xbar.png is still broken. How do I specify this properly, or does it only show up on master?

WRoenninger · 2021-02-01T12:52:02Z

Is there a possibility to check the pipeline status of a failing gitlab-ci? Just to check if the ci failed on a stall due to a functional failing module, or that the timeout time was not long enough and the ci was still simulating.

andreaskurth · 2021-02-02T15:16:19Z

Seems however that the link to the .png in line 28 to docs/axi_xbar.png is still broken. How do I specify this properly, or does it only show up on master?

You need to remove the docs/ prefix; the relative URL is simply axi_xbar.png.

Is there a possibility to check the pipeline status of a failing gitlab-ci? Just to check if the ci failed on a stall due to a functional failing module, or that the timeout time was not long enough and the ci was still simulating.

Only if you have access to the mirrored repository on which CI runs. For this branch, the timeout was too low, because the crossbar simulated for more than 3 hours. -- Why is that?

WRoenninger · 2021-02-03T15:59:19Z

Why is that?

The simulation script currently has 5 different parameters which each get checked for two values in the script. This equates to 32 simulation runs. In the tb each instantiated rand_master completes 100 read and 100 write bursts. However the wider tb configurations run quite slow as there is a lot going on (e.g. 6 slv_ports, 9 mst_ports). Adding new the EnableAtops parameter doubled the runtime.

I could tune down the numbers though, e.g. to less slv_ports. I suspect they tend to slow down the simulation quite a bit.

Would it be reasonable to split the script for axi_xbar? So that there is not only one axi_xbar thread in the ci, but multiple. However I think the simulate script is currently not set up for this in a scalable / non-hacky manner.
Also adding the internal pipeline from #116 will again double the number of simulations to be run by the ci.

Also similar issue will be in #33, there are at least 5 parameters which should be checked in range by the ci.

The documentation for `axi_xbar` is now auto generated from the source file. The diagram has been moved to docs:. It is no longer necessary to keep these files.

WRoenninger · 2021-02-04T16:27:22Z

Just seen: As is scripts/axi_intercon_gen.py will be broken after implementing these changes.

WRoenninger requested review from andreaskurth and zarubaf January 26, 2021 11:04

andreaskurth approved these changes Jan 29, 2021

View reviewed changes